Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify HTTP/2 setting parameter reservation #3955

Merged
merged 5 commits into from
Sep 1, 2020

Conversation

MikeBishop
Copy link
Contributor

Fixes #3954 by copying the language we already had for frame types and taking @martinthomson's suggestion from #3942.

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -http labels Jul 23, 2020
Copy link
Contributor

@bencebeky bencebeky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -1395,6 +1395,10 @@ settings to have any meaning upon receipt.
Because the setting has no defined meaning, the value of the setting can be any
value the implementation selects.

Setting identifiers which were used in HTTP/2 where there is no corresponding
HTTP/3 setting have also been reserved ({{iana-settings}}). These settings MUST
NOT be sent, and receipt MAY be treated as an error of type H3_SETTINGS_ERROR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MAY here could lead to confusion down the road. Is there any reason for us to not be prescriptive here? I think it would be safer to clearly have either MUST treat as error or MUST ignore here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've used MUST NOT send / MAY error elsewhere when we haven't wanted to mandate checking. It's enough of a deterrent to the bad behavior, when the requirement to treat as an error isn't vital to interop. That said, it also isn't difficult to do the check, so if there's consensus to require it, I'm not opposed to the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be in favor of MUST treat as error. This isn't hard to detect, and it seems quite reasonable to close the connection on such errors.

@@ -2316,20 +2320,26 @@ SETTINGS_HEADER_TABLE_SIZE:

SETTINGS_ENABLE_PUSH:
: This is removed in favor of the MAX_PUSH_ID which provides a more granular
control over server push.
control over server push. Specifying a setting with the identifier 0x2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead phrase these as normative? For example: Endpoints MUST NOT send a setting with identifier 0x2 (corresponding to the SETTINGS_ENABLE_PUSH parameter) in the HTTP/3 SETTINGS frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing in this appendix is normative, by design. The normative requirement is in the body of the document, and this appendix simply notes that there is a prohibition. I could add a reference to the prohibition, if that would help.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reference to the prohibition works for me, I just want to make sure we have easily-discoverable text clearly stating that endpoints MUST NOT send those

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...with or without David's changes.

@@ -1395,6 +1395,10 @@ settings to have any meaning upon receipt.
Because the setting has no defined meaning, the value of the setting can be any
value the implementation selects.

Setting identifiers which were used in HTTP/2 where there is no corresponding
HTTP/3 setting have also been reserved ({{iana-settings}}). These settings MUST
NOT be sent, and receipt MAY be treated as an error of type H3_SETTINGS_ERROR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
NOT be sent, and receipt MAY be treated as an error of type H3_SETTINGS_ERROR.
NOT be sent, and their receipt MAY be treated as an error of type H3_SETTINGS_ERROR.

@@ -1395,6 +1395,10 @@ settings to have any meaning upon receipt.
Because the setting has no defined meaning, the value of the setting can be any
value the implementation selects.

Setting identifiers which were used in HTTP/2 where there is no corresponding
HTTP/3 setting have also been reserved ({{iana-settings}}). These settings MUST
NOT be sent, and receipt MAY be treated as an error of type H3_SETTINGS_ERROR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be in favor of MUST treat as error. This isn't hard to detect, and it seems quite reasonable to close the connection on such errors.

@MikeBishop
Copy link
Contributor Author

@LPardue points out that SETTINGS and frames are different, because of tunneling intermediaries. The intermediary need not check every received frame for validity if they're not processing the frames. SETTINGS, on the other hand, have to be checked because they're inherently hop-by-hop. While I'm not crazy about having SETTINGS and frames be different, there is at least a justification for it.

@MikeBishop MikeBishop added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Jul 28, 2020
@martinthomson
Copy link
Member

I'm sorry @MikeBishop, that makes no sense. You can't pick and choose which parts of a protocol you understand.

@janaiyengar
Copy link
Contributor

@MikeBishop : Doesn't that argument then argue for MUST close the connection on error? What am I missing?

@martinthomson martinthomson removed the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Aug 4, 2020
@MikeBishop
Copy link
Contributor Author

@martinthomson, that goes back to the discussion we had around stream vs. connection errors. @kazuho's point, if I can summarize briefly, is that an intermediary might directly copy some things from its connection to the client onto its connection to the server, or vice versa. That led to the logic of using connection errors for things definitely generated by your peer; settings are definitely generated by the peer, which argues for "MUST error," while frames might have come from upstream.

However, I note that we currently don't specify whether receiving a reserved frame type is a stream or a connection error, so I've opened #3991 for that.

@martinthomson
Copy link
Member

I'm going to argue that frames also need to definitely come from a peer. If SETTINGS definitely comes from an immediate peer, then transport-level intermediation is not valid. That is, you forward (HTTP) frames blindly at your own risk.

That is, if you indicate in SETTINGS that you understand an extension and then blindly forward frames related to that extension, you are responsible for the effects.

@MikeBishop
Copy link
Contributor Author

When you argue that, do so on #3991 and we'll invite @kazuho for the cage match. For this issue/PR, it sounds like we have violent agreement that SETTINGS are a MUST-connection-error.

@janaiyengar janaiyengar merged commit 73a5557 into master Sep 1, 2020
@janaiyengar janaiyengar deleted the http/h2_settings_reserved branch September 1, 2020 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/2 Settings reservation isn't called out
7 participants